Skip to content

Conversation

@tbezman
Copy link
Contributor

@tbezman tbezman commented Sep 17, 2021

Hoping this a welcome addition. I added prettier and setup a config file that closely matches the existing code style that already existed. I'm also only checking index.ts for now which seems like one of our only TypeScript source files.

I also setup the repo's first Github Action to ensure code is properly formatted from people posting PRs.

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable configuration. Do we need to keep this consistent as other NEAR JavaScript Libraries' setup (if any)? @volovyk-s

@volovyks
Copy link
Contributor

Wow, Awesome contribution @tbezman ! We definitely need to add GitHub actions here.
However, we are already using eslint across our projects. We have lint and fix commands in Borsh JS package.json. eslint is a linter, so it's more powerful and can look for potential errors in code. But I can agree that Prettier is doing a better job in code formatting. So I can see two ways how we can stay consistent and make this project better.

  1. (Easy) Use eslint instead of Prettier in added GitHub Action.
  2. (Hard) Use both eslint and Prettier. They can be in conflict, so we will need to read this article first. Maybe we will need to have two separate actions for them.

I would rather use default parameters for Prettier and not override them with a separate config file. More chances that it will not be in conflict with text editors and IDEs.

Bonus points for adding Unit tests GitHub Action (just run yarn test)

@tbezman
Copy link
Contributor Author

tbezman commented Sep 17, 2021

Thanks for getting back to me. I'll apply all of this feedback ASAP.

@tbezman
Copy link
Contributor Author

tbezman commented Sep 18, 2021

@volovyk-s Yeah there is a config we can use to disable all eslint rules around code formatting so we can use prettier for formatting, and eslint for code conventions. I think I'll save the ESLint configuration for an upcoming PR since we have a bunch of warnings atm. I'll do the eslint stuff as a followup.

@tbezman
Copy link
Contributor Author

tbezman commented Sep 18, 2021

Okay @volovyk-s, I updated the PR. I removed the prettier config (now we're using prettier defaults), and added unit tests to the github action workflow. I think you'll have to make the action a required check on PRs after this merges.

Copy link
Contributor

@volovyks volovyks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @tbezman! And thank you for the tests.
NP: When I run yarn pretty:check I'm not getting any errors, but yarn pretty command generates some formatting changes. Can you please run it and push changes?

@tbezman
Copy link
Contributor Author

tbezman commented Sep 20, 2021

@volovyk-s On this branch, when I run yarn pretty, none of my files actually change but I do get some output in the console about which files prettier ran against. Are you seeing the same behavior or is something borked?

@volovyks volovyks merged commit 9ed926b into near:master Sep 21, 2021
@volovyks
Copy link
Contributor

@tbezman I have double-checked. All good, thank you!

@volovyks
Copy link
Contributor

Hm, it's getting changed after yarn test, that is weird :/

@volovyks
Copy link
Contributor

Test and pretty (with small fix) GitHub actions works!
#35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants